fix(sei-global-wallet): harden consumer integrations - #343

[blocker] Exact-pinning viem as a peer dependency will break installs for most consumers.

This goes from ^2.7.12 to exactly 2.45.3. peerDependenciesMeta.optional only means npm won't auto-install it and won't error when it's absent — if the consumer already has viem (which nearly every wagmi/RainbowKit/ConnectKit dApp does) at any version other than 2.45.3, npm 7+ fails the install with ERESOLVE could not resolve / conflicting peer dependency. Since 2.45.3 is older than what most apps are on, this is a forced downgrade, not a floor.

The same applies to the other newly-exact peers in this block: @dynamic-labs/ethereum-aa: 4.96.3 (L43), @solana/web3.js: 1.98.1 (L45), @zerodev/sdk: 5.5.7 (L49), and zksync-sso: 0.2.0 (L51).

The stated reason for the viem pin (newer viem/Ox pulling node:worker_threads into Vite resolution) is a consumer bundler-resolution problem; a library peer range shouldn't be the mechanism that dictates it. A bounded range that excludes the known-bad versions — e.g. >=2.7.12 <2.55.0, or ^2.45.3 if the issue is actually fixed forward — keeps the constraint expressible without making every non-matching consumer install fail. Applying the same treatment to the other four would keep this a non-breaking release.

[blocker] Exact-pinning Dynamic prevents consumers from ever receiving the upstream fix. (This is Codex's finding, with one addition.)

Codex is right that 4.96.3 knowingly ships vulnerable axios@1.16.0 and uuid@11.1.0, and that a library's overrides do not propagate — so every downstream npm audit fails unless the app author reads the README and copies the override block by hand.

The part worth adding: the previous ^4.60.1 at least let consumers pick up 4.96.4+ the moment Dynamic corrects those transitive pins. The exact 4.96.3 removes that escape hatch entirely — unblocking consumers then requires a new @sei-js/sei-global-wallet release and a coordinated upgrade across every downstream dApp.

Given the whole waiver is explicitly described as temporary, a caret or a tilde (~4.96.3) would preserve the reproducibility this PR is after while letting the fix flow through automatically. If exactness is truly required for the metafile/lockfile assertions in scripts/check-sei-global-wallet.ts, that's an argument for pinning in the repo lockfile, not in the published manifest.

[suggestion] Installing a global process shim has app-wide blast radius, not just Dynamic's.

This module is loaded as an import side effect of every entrypoint, including the one-liner import '@sei-js/sei-global-wallet/eip6963' the README tells apps to put at startup. In a real browser or edge runtime process is genuinely undefined, so this installs process/browser.js globally — which every subsequently-loaded library then sees.

The concrete hazard: process/browser.js sets process.env = {}, so process.env.NODE_ENV is undefined. Libraries that gate on typeof process !== 'undefined' && process.env.NODE_ENV !== 'production' will take their development branch (extra warnings, unoptimized paths) in a production bundle. The global = globalThis alias above has the mirror problem: libraries that sniff Node via typeof global !== 'undefined' (buffer, readable-stream, bn.js and friends all do variants of this) will misdetect the environment.

The "don't clobber consumer values" guard is good and handles the case where someone already shimmed. The gap is the case where nothing is defined on purpose. A couple of options that keep the fix but shrink the radius: set NODE_ENV: 'production' on the shim's env, or install a minimal object with only the fields Dynamic actually reads rather than the full browserify shim.

[suggestion] I verified this is correct — the literal ends in Pg== (2 bytes, >), and slice(0, -2) + 'o=' yields Pgo= (2 bytes, > \n). So the terminal newline does land, and the sha256 in the spec will match.

That said, this is very hard to audit: a reader has to hand-decode base64 to confirm the constant is what the comment claims, and it silently depends on the literal above ending with exactly two = padding chars. Any future re-encode of the icon changes that alignment and this line corrupts the data URI instead of failing loudly.

Since you already know the exact target bytes, encoding the trailing newline directly into the literal on L22 and deleting this line removes the coupling entirely. It also lets config stay immutable — right now the exported object is mutated after definition, which is easy to miss when reading config.ts top-down.

[suggestion] This asserts that the vulnerable versions are present, so the check goes red precisely when the underlying problem gets fixed upstream. The moment Dynamic corrects its transitive pins, this line (and L589's uuid companion, plus assert.notEqual(result.exitCode, 0, ...) at L488) fails and someone has to debug a CI failure that actually represents good news.

Inverting these to tolerate improvement — assert the audit is clean or that the only findings are within the accepted set — would make the job self-healing. If you want to keep the affirmative signal that the waiver is still needed, console.warn on "waiver no longer required" reads much better than a hard failure.

[suggestion] assert.deepEqual(advisories, [...acceptedBunAdvisories].sort()) requires the advisory set to match exactly — no more, no fewer. This couples a PR-blocking check to the GitHub Advisory Database, which changes independently of this repo. A brand-new advisory published against any dep in the Bun tree turns an unrelated PR red, and so does an advisory being withdrawn.

A subset assertion ("every reported advisory is in the accepted set") gives you the same protection against new exposure without failing on the withdrawal case. The README already promises "the verifier fails if Bun reports any new, missing, or different advisory", so this would need a wording tweak there too.

[nit] This only inspects viteBuild.stderr. Vite/Rollup routes plugin warnings — including the "...has been externalized for browser compatibility" message this regex is hunting for — through its own logger, which writes to stdout in most versions. Worth matching against ${viteBuild.stdout}${viteBuild.stderr} so the check can't silently pass.

[suggestion] events isn't imported anywhere — I grepped packages/sei-global-wallet/src and scripts/check-sei-global-wallet.ts and found no reference. process is used (browserGlobal.ts imports process/browser.js), but events looks like it was added speculatively alongside it.

Unused runtime deps are worth removing rather than leaving in: this one ships to every consumer's install tree and adds a package to their audit surface for no benefit. If it's actually needed to satisfy a bundler resolution that the smoke test covers, a comment in the changeset explaining that would help, since it isn't discoverable from the source.

[nit] The new mark is a solid #000000 fill on a transparent background, replacing the #9E1F19 wordmark. Per REVIEW_GUIDELINES §2 I'm not disputing the asset itself — the source is cited and the sha256 is pinned, which is exactly the right treatment.

The usability question is separate: EIP-6963 icons render in wallet-selection modals, and RainbowKit / ConnectKit / Web3Modal all default to dark surfaces. A pure-black transparent SVG will be close to invisible there. Most wallets ship an icon with its own background fill for this reason. Worth checking against a dark modal before release; if the brand kit has a variant with a background plate, that would be the safer pick.

[suggestion] patch understates this. The package is at 1.4.1, so semver applies, and tightening optional peer ranges to exact versions (see the package.json comment) means consumers who upgrade within a patch range can hit ERESOLVE on their next install. A dependency-resolution break is the kind of thing a patch bump specifically promises won't happen.

If the exact peer pins stay, this should be major. If they're relaxed to bounded ranges, minor fits the new exports (registerEIP6963Provider, unregisterEIP6963Provider, eip6963ProviderInfo, registerSolanaStandard) nicely.

[blocker] Exact viem peer pin is stricter than the contract it claims to mirror, and will break consumers.

The changeset says this release "mirror[s] its optional peer contract" and the README calls this "the compatible Viem floor", but Dynamic 4.96.3 declares "viem": "^2.45.3" (visible in the bun.lock hunk for @dynamic-labs/global-wallet-client). 2.45.3 is not a floor — it is an exact pin, and it is narrower than upstream.

Every other exact peer here (@dynamic-labs/ethereum-aa@4.96.3, @solana/web3.js@1.98.1, @zerodev/sdk@5.5.7, zksync-sso@0.2.0) does match Dynamic exactly. viem is the one deviation, and it's the one peer nearly every consumer already has: any app on wagmi/RainbowKit/ConnectKit will be on some viem 2.x newer than 2.45.3 and will hit ERESOLVE on npm install (optional peers still conflict once the package is actually present).

The stated motivation — 2.55.19 pulling Ox Tempo's node:worker_threads into Vite resolution — is a property of the harness's own bundle, not something that warrants forcing a global downgrade on every downstream dApp. Suggest "viem": "^2.45.3" here to match upstream, and keep the exact 2.45.3 in devDependencies (line 39) and in fullConsumerManifest so the smoke test still exercises the known-good graph.

[blocker] patch under-states this change for a published package.

This release narrows or relocates the peer contract in ways that can fail a consumer's install or type resolution:

• viem: ^2.7.12 → 2.45.3
• @dynamic-labs/ethereum-aa: ^4.15.0 → 4.96.3
• @zerodev/sdk: 5.4.36 → 5.5.7
• @solana/web3.js: ^1.92.1 → 1.98.1
• zksync-sso@0.2.0 added as a new (optional) peer
• @wallet-standard/wallet moved out of dependencies into optional peerDependencies — correct, since nothing in src/ imports it, but it does stop being installed transitively

A consumer taking this as a patch bump via ^1.4.1 can get an install that no longer resolves. .changeset/config.json uses fixed: [] / linked: [], so bumping just this package to minor (or major) costs nothing elsewhere.

[suggestion] events@3.3.0 is declared as a runtime dependency but nothing under packages/sei-global-wallet/src/ imports it (process/browser.js is the only shim actually imported, in browserGlobal.ts:1). Unless this is deliberately present so a bundler can resolve a transitive bare events specifier — in which case it needs a comment saying so, because it looks removable — it ships an unused package to every consumer. Please drop it or document why it's load-bearing.

[suggestion] The job runs test:sei-global-wallet-release, which imports scripts/dynamic-package-lock.js (check-sei-global-wallet.ts:8) for the stale-Dynamic-4.96.1 graph assertion — but neither scripts/dynamic-package-lock.ts nor its test is in this path filter. A PR that changes the dependency-graph enforcement therefore skips the very check it modifies. Add - 'scripts/dynamic-package-lock*' alongside this entry.

(Raised by Codex; confirmed.)

[suggestion] This installs a process shim onto the page's global object as a side effect of importing any entrypoint — dynamicClient, dynamicEthereum, dynamicFeatures, dynamicSolana, and dynamicZerodev all import './browserGlobal.js', so merely import '@sei-js/sei-global-wallet' mutates the consumer's global scope in a browser.

process/browser.js sets process.browser = true and process.env = {}. Any other library on the page that branches on typeof process !== 'undefined' to decide "I'm in Node" now takes the wrong branch, and reads of process.env.X that previously threw now silently yield undefined. The typeof === 'undefined' guard and writable: true are good mitigations, but they don't help a library that loads after this one.

If Dynamic only needs these on its own module scope, prefer scoping the shim (bundler define/inject documented for consumers, or a wrapper that passes them in). If a true global really is required, the README's "All entrypoints are ESM-only…" paragraph should state plainly that importing the package defines globalThis.process and globalThis.global when absent.

[suggestion] I verified this is correct — the literal's final base64 group is Pg== (one byte, >), and slice(0, -2) + 'o=' rewrites it to Pgo= (> + \n) — but the correctness depends on an invariant that is invisible at the call site: the literal must decode to a length ≡ 1 (mod 3). Re-encode the SVG with any other trailing byte and this silently corrupts the icon instead of failing loudly.

Since the base64 is already a single-line string literal, the stated reason ("instead of relying on editor treatment of a multiline asset") doesn't apply. Just embed the final value ending in …c3ZnPgo= and delete the post-hoc mutation; the SHA-256 assertion in config.spec.ts already pins the result either way.

[suggestion] assert.notEqual(result.exitCode, 0, ...) plus the exact-set assertions below make the smoke job fail whenever the Bun advisory set changes in either direction — including when upstream finally fixes bn.js/ws (the good outcome) or when GitHub publishes any new advisory for the AA subtree. The failure will land on whichever unrelated wallet PR happens to run next, and the message ("unexpectedly reported a clean dependency tree") won't obviously read as "the waiver can be removed now."

At minimum, make the clean-tree message actionable ("advisories resolved upstream — drop the waiver from README.md and this script"). Better: assert the accepted set is a superset check with a warning on shrinkage, and move the strict equality to a scheduled job.

[nit] STALE_DYNAMIC_VERSION hardcodes 4.96.1, so this guard only ever detects that one release. As soon as Dynamic ships 4.96.4 or 4.97, a genuinely stale nested subtree passes silently. Consider asserting the inverse instead — every @dynamic-labs/* location resolves to the expected pinned version — which stays meaningful across upgrades.

[nit] "sideEffects": true is equivalent to the previous state (field absent), so nothing regresses. But since only eip6963.js, solana.js, and lib/browserGlobal.js actually have side effects, an array — ["./dist/eip6963.js", "./dist/solana.js", "./dist/lib/browserGlobal.js"] — would let bundlers tree-shake the root entry for consumers who only pull a couple of named exports.

Typecheck ignores consumer failures

Medium Severity

assertDeclarationsResolveWithoutOptionalPeers runs tsc with failures allowed and only treats diagnostics whose path matches @sei-js/sei-global-wallet/dist/ as failures. Errors attributed to strict-types.ts itself—missing exports, unresolved subpaths, or other public-API type breaks—leave ownDiagnostics empty, so the assert passes and the release verifier can green-light broken consumer typings.

 

^{Reviewed by Cursor Bugbot for commit 8428369. Configure here.}

[nit] @wallet-standard/wallet is now declared in both dependencies (line 28) and devDependencies. npm and Bun resolve the runtime entry and ignore this one, so it is a no-op today, but it is a drift hazard: bumping only this line silently changes nothing, and bumping only line 28 leaves a stale-looking dev range behind. Drop the devDependency entry.

[suggestion] events is declared as a runtime dependency of this package purely so a different package (@zerodev/sdk) can resolve its bare events import — the README says as much. That only works under hoisted layouts. Under pnpm's default isolated node_modules or Yarn PnP, events installed for @sei-js/sei-global-wallet is not on @zerodev/sdk's resolution path, so the documented Could not resolve "events" failure still occurs. The verifier only exercises npm and Bun, both hoisted, so this gap is untested.

It also ships to every consumer of the root / ./eip6963 / ./ethereum entrypoints, none of which touch ZeroDev. Suggest documenting the pnpm/PnP caveat in the "Optional peer versions" section and pointing those users at a bundler alias, since the dependency can't be a general fix.

[suggestion] This mutates the process/browser.js module singleton, not a private copy. The install guard is typeof globalThis.process === 'undefined', but a consumer can be using process/browser.js locally — e.g. webpack's ProvidePlugin: { process: 'process/browser.js' }, which is the standard setup and does not define globalThis.process. In that configuration this branch is taken, and the consumer's own process object gets NODE_ENV forced to production even in a development build, so their libraries take production branches.

The README's guarantee ("neither is installed when the consumer or runtime already defines it") holds for the defineProperty on line 32, but not for this line — the mutation escapes the global check. Consider setting NODE_ENV on a shallow copy of the shim rather than the shared module object:

const shim = { ...processShim, env: { NODE_ENV: 'production', ...processShim.env } };
install('process', shim);

[suggestion] The full npm consumer asserts a hard total === 0 against the live advisory database. The Bun path deliberately tolerates drift (subset check, "reports rather than fails when one stops being reported") precisely so that "a withdrawn or upstream-fixed advisory must not fail an unrelated pull request" — but the reverse case isn't handled here: a newly published advisory anywhere in the viem/@solana/web3.js/@zerodev/sdk tree turns the daily cron and every wallet-touching PR red with no change to this repo.

Since this workflow is path-triggered on bun.lock and package.json, that will surface on changes unrelated to the wallet. Consider giving npm the same accepted-advisory-set treatment (empty set today), so a new finding produces an actionable diff against a declared baseline rather than an opaque failure.

[nit] The regex is case-insensitive (/gi) but membership is tested case-sensitively against acceptedBunAdvisories (lowercase) via includes on line 588 and reported.has on line 596. If bun audit ever emits an ID with different casing, the same advisory would both fail the unwaived check and be reported as "fixed upstream". GHSA IDs are lowercase in practice, so this is theoretical — normalizing with .map((id) => id.toLowerCase()) here removes the coupling.

[nit] registeredWallet is memoized for the module lifetime with no way to clear it, which is asymmetric with the EIP-6963 path in this same PR (unregisterEIP6963Provider). A consumer that can tear down and re-register the EIP-6963 provider has no equivalent for wallet-standard, and HMR will keep handing back a wallet bound to a stale Wallet client. Consider an unregisterSolanaStandard() that resets this, for parity.

[nit] Not a regression — this is the same value the previous code passed as uuid via config.environmentId — but the changeset bills it as a fix ("use stable UUIDv4 provider metadata"), and EIP-6963 actually specifies the opposite: uuid must "uniquely distinguish different EIP-1193 provider sessions ... during the lifetime of the page". A constant is common in the wild and low-risk, but it interacts with the new cleanup helpers: unregisterEIP6963Provider() followed by registerEIP6963Provider() creates a fresh createEIP1193Provider(Wallet) instance announced under the same uuid, which a discovery UI keying on uuid may treat as the old session. Worth either generating per-registration with crypto.randomUUID(), or rewording the changeset so it doesn't read as a spec-conformance fix.

[nit] "sideEffects": true is correct and conservative, but it's also the bundler default when the field is absent, so it buys nothing while ruling out tree-shaking entirely. An array would be more useful: the side effects live in dist/eip6963.js, dist/solana.js, and dist/lib/browserGlobal.js, so listing those would let bundlers drop unused re-exports from dist/index.js for consumers that only use a subpath.

[suggestion] These assertions pin exact versions that this repo does not control and that are not held by any lockfile — the consumer dirs are installed fresh from the live registry.

• node_modules/bn.js resolves 5.2.5 from Solana's ^5.x range → a bn.js@5.2.6 release turns this red.
• node_modules/jayson/node_modules/ws resolves 7.5.13 from Jayson's ^7.5.10 → same for ws@7.5.14.
• Same pattern at lines 813–817 for the Bun lock (bn.js@5.2.5, jayson/ws@7.5.13).

Since this workflow gates PRs and runs on a daily cron, an unrelated upstream patch release will fail main and every open PR touching packages/sei-global-wallet/**. The lines that are pinned by your overrides (node_modules/ws = 8.21.0, the scoped bn.js = 4.12.5, viem's ws = 8.18.3 under the pinned viem@2.45.3) are stable and worth keeping exact. For the floating ones, asserting the intended invariant instead — major/minor line, or "Jayson stayed on ws 7 while the hoisted copy is ws 8" — keeps the check meaningful without the drift.

[suggestion] Installing a page-wide process with NODE_ENV=production is a large blast radius for a side effect of import '@sei-js/sei-global-wallet'. Two consequences beyond Dynamic:

1. typeof process !== 'undefined' is the most common Node-vs-browser sniff. After any entrypoint of this package loads, every library initialised afterwards sees process defined. (process/browser.js has no versions, so the stricter process.versions?.node checks are unaffected — the loose ones are not.)
2. A consumer whose dev bundle reads NODE_ENV at runtime rather than through a bundler define will now take its production branch during development — losing React's dev warnings, for example.

The README documents both and the "don't overwrite what the consumer set" guard is the right shape, so this isn't a defect. But if Dynamic only needs these on its own module scope, scoping the shim there rather than to globalThis would avoid changing behaviour for unrelated libraries in the host app.

[nit] @wallet-standard/wallet is already a runtime dependency at line 28, so this devDependencies entry is a no-op. Worse, the two ranges can silently diverge on a future bump and only the dependencies one will affect consumers. Suggest dropping this line — entrypoints.spec.ts already asserts the dependencies entry is the load-bearing one.

[suggestion] The @modelcontextprotocol/sdk floor is still ^1.17.5 (line 36), which predates the SDK accepting zod 4 — the versions that do declare "zod": "^3.25 || ^4.0" are considerably later (your lockfile resolves 1.30.0). npm/bun will normally pick the newest 1.x, so this repo is fine and mcp-zod-lock.test.ts guards it. But a consumer who pins the SDK anywhere in 1.17.x–1.1x.x gets zod 4 hoisted for @sei-js/mcp-server and a nested zod 3 under the SDK; the instanceof ZodType checks in server.tool/server.prompt then fail against schemas built by the other copy. Raising the SDK floor to the first release that declares zod-4 support makes the pairing unresolvable rather than silently broken.

[nit] Two small things on the new uuid:

1. It is byte-identical to environmentId above. That is unchanged behaviour (the old code passed config.environmentId as the EIP-6963 uuid), but as two independent literals they can now drift apart without anything noticing. A comment saying they are intentionally the same value — or deriving one from the other — would make the coupling explicit.
2. EIP-6963 specifies uuid as a UUIDv4 that distinguishes provider sessions with otherwise-matching properties "during the lifetime of the page", i.e. generated per instance rather than baked in. A constant is what most wallets ship in practice and is harmless with a single provider instance, so I'd leave it — just flagging that "stable UUIDv4" in the changeset is a deliberate deviation from the spec's wording, not conformance to it.

[nit] Two loose edges in the waiver check, both low-probability:

• This regex scans the whole serialised report, so an advisory on an unrelated package whose name or title merely contains uuid (@lukeed/uuid, uuid-parse, …) fails the run with a misleading "overrides stopped taking effect" reading. Checking for axios/uuid as top-level keys of the parsed report would say exactly what you mean.
• reported (line 580) is built only from GHSA- matches, so an advisory that surfaced without a GHSA id would be invisible to the unwaived assertion and silently pass. Bun's audit output does carry GHSA URLs today, so this is defensive rather than a live gap — but a count check (reported.size vs. the number of findings in the report) would close it cheaply.